Skip to content

feat: baseline run:script, run:container and run:shell to support stdin and argv #1129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 25, 2025
Merged

feat: baseline run:script, run:container and run:shell to support stdin and argv #1129

merged 1 commit into from
Nov 25, 2025
+146 −35

Conversation

@hirenr
Copy link
Contributor

@hirenr hirenr commented Nov 11, 2025 •
edited
Loading

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Use Cases
  • Community
  • CTK
  • Other

Discussion or Issue link:
Slack Discussion Link

What this PR does:
Update the run:* syntax to have a unified structure. Introduced parameters stdin, and updated arguments.

arguments as string[] passed as CLI args to run:shell / run:container / run:script underlying command
Pass stdin string via a new string property to the run:shell / run:container / run:script underlying command!

Additional information:
The advantages to the following are:

  • No injection of code into scripts (for input args etc) and other underlying code!
  • Makes the inline/referenced scripts fully portable without relying on the runtime
  • Execution and passing of data to scripts similiar, with how it would work outside of a workflow environment. For eg. when running the script directly via node command.
  • Input (via STDIN) and Output (via STDOUT, STDERR, CODE) more inline with each other.

@hirenr
Copy link
Contributor Author

hirenr commented Nov 11, 2025

ref: #1128

@cdavernas cdavernas requested a review from Copilot November 11, 2025 22:37
Copilot AI reviewed Nov 11, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces unified support for stdin and arguments parameters across run:container, run:script, and run:shell tasks. The change standardizes how data is passed to these tasks by enabling input via standard input and command-line arguments, rather than injecting data into scripts.

Key Changes:

  • Changed arguments from object type to string[] array for all three run task types
  • Added new stdin property (type string) to pass data as standard input for all three run task types
  • Updated examples to demonstrate the new unified syntax

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
schema/workflow.yaml Updated schema definitions for container, script, and shell tasks to support stdin string property and changed arguments from object to string array
examples/run-shell-stdin-and-arguments.yaml New example demonstrating shell task with stdin and arguments
examples/run-script-with-stdin-and-arguments.yaml New example showing script task using stdin, arguments, and environment variables
examples/run-script-with-arguments.yaml Removed old example that used object-based arguments (replaced by new unified syntax)
examples/run-container-stdin-and-arguments.yaml New example demonstrating container task with stdin and arguments
dsl-reference.md Updated documentation for all three run task types to reflect new stdin property and array-based arguments

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ricardozanini
Copy link
Member

ricardozanini commented Nov 12, 2025

Can youn take a look at the CI error?

@hirenr
Copy link
Contributor Author

hirenr commented Nov 13, 2025

Fix added for CI error!

@cdavernas cdavernas requested a review from Copilot November 13, 2025 12:38
Copilot AI reviewed Nov 13, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cdavernas
cdavernas approved these changes Nov 13, 2025
View reviewed changes
Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Cheers @hirenr ❤️

@ricardozanini
Copy link
Member

ricardozanini commented Nov 14, 2025

@hirenr @cdavernas the merge is blocked because there's no verified signatures.

@hirenr you must have a GPG key locally and verify your signature accordingly, then you can squash and sign your commits and force push again so I can merge it. We can manually approve the DCO to get a green, but github/CNCF rules won't allow it if you don't have a local GPG key.

@hirenr
Copy link
Contributor Author

hirenr commented Nov 24, 2025

@ricardozanini I think its done!! I have signed with my gpg key!

@ricardozanini
Copy link
Member

ricardozanini commented Nov 24, 2025

@hirenr see:

There are 2 commits incorrectly signed off. This means that the author(s) of these commits failed to include a Signed-off-by line in their commit message.

To avoid having PRs blocked in the future, always include Signed-off-by: Author Name <[email protected]> in every commit message. You can also do this automatically by using the -s flag (i.e., git commit -s).

Here is how to fix the problem so that this code can be merged.

Can you please squash your commits and sign it? Then, I believe we will be set!

@hirenr
Copy link
Contributor Author

hirenr commented Nov 25, 2025

DCO should be green!! I added a new manual fix (and removed the offending commits by copilot).

@ricardozanini
Copy link
Member

ricardozanini commented Nov 25, 2025

@hirenr the PR is still blocked. I think squashing and signing is the way to go. After squashing, you will have only one commit in the tree, signed and set. Then you force push, and DCO will let us merge.

Screenshot 2025-11-25 at 9 11 51 AM

@ricardozanini ricardozanini merged commit 8954d83 into serverlessworkflow:main Nov 25, 2025
3 checks passed
@ricardozanini
Copy link
Member

ricardozanini commented Nov 25, 2025

Many thanks, @hirenr!

@hirenr hirenr deleted the pr-1128 branch November 25, 2025 17:26
@mrsimonemms mrsimonemms mentioned this pull request Nov 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@ricardozanini ricardozanini ricardozanini approved these changes

@cdavernas cdavernas cdavernas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hirenr @ricardozanini @cdavernas